Skip to content

Add React Router plugin performance optimizations#39

Open
ScriptedAlchemy wants to merge 33 commits into
mainfrom
perf/bundling-performance
Open

Add React Router plugin performance optimizations#39
ScriptedAlchemy wants to merge 33 commits into
mainfrom
perf/bundling-performance

Conversation

@ScriptedAlchemy

@ScriptedAlchemy ScriptedAlchemy commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add repeatable plugin performance benchmark workflows and synthetic fixtures.
  • Cache route module/export/chunk analysis and route artifact generation to reduce repeated transform work.
  • Replace Babel/esbuild transform internals with Yuku parser/codegen/analyzer while keeping cached route artifact APIs.
  • Add an opt-in lazyCompilation plugin option that forwards to Rsbuild's dev-only dev.lazyCompilation config when explicitly set.
  • Abort internal build-time requests after consuming prerender/SPA responses so React Router stream timeout handles do not keep build processes alive.

Performance

Latest local benchmark highlights:

Default profile, median wall time vs original baseline

synthetic-256-ssr-esm        1.78s -> 1.26s  1.41x faster
synthetic-256-ssr-esm-split  2.30s -> 1.69s  1.36x faster
synthetic-256-spa            6.70s -> 1.23s  5.47x faster
synthetic-256-sourcemaps     1.81s -> 1.36s  1.33x faster

total                       12.59s -> 5.54s  2.27x faster

Yuku microbench after consolidation:

BENCH_ITERATIONS=100 BENCH_SAMPLES=24 pnpm bench:yuku

total 184.64ms -> 39.69ms = 4.65x faster

Lazy compilation note:

  • lazyCompilation is dev-only in Rsbuild.
  • It remains opt-in because React Router route modules must be synchronously available during hydration.
  • Production build benchmark showed no production build speedup, as expected.

Test plan

  • pnpm test:core tests/index.test.ts tests/remove-exports.test.ts
  • pnpm build
  • CI=true pnpm --filter './examples/client-only' test:e2e
  • pnpm test
  • pnpm test tests/index.test.ts tests/benchmark-fixture.test.ts
  • pnpm bench --profile default --filter synthetic-256-spa --iterations 5 --warmup 1 --clean build --format both --out .benchmark/results/spa-abort-request-final
  • REACT_ROUTER_BENCHMARK_LAZY_COMPILATION=1 pnpm bench --profile default --filter synthetic-256-ssr-esm --iterations 5 --warmup 1 --clean build --format both --out .benchmark/results/lazy-on-ssr-esm

@ScriptedAlchemy ScriptedAlchemy changed the title Add React Router plugin performance benchmarks Add React Router plugin performance optimizations Jun 18, 2026
@pkg-pr-new

pkg-pr-new Bot commented Jun 18, 2026

Copy link
Copy Markdown

Open in StackBlitz

pnpm add https://pkg.pr.new/rsbuild-plugin-react-router@13dd705

commit: 13dd705

ScriptedAlchemy and others added 4 commits June 18, 2026 06:22
Document the performance optimization and benchmark tooling updates for release.
Co-authored-by: Matthew Davis <matthewdavis@openai.com>
@ScriptedAlchemy ScriptedAlchemy marked this pull request as ready for review June 18, 2026 22:18

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f28c70be5b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/route-chunks.ts Outdated
@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR replaces the @babel/* AST toolchain with yuku-parser, yuku-codegen, and yuku-analyzer across route-analysis and plugin-utility modules. Export name discovery and route-module analysis now use bounded memoization caches keyed by resource path and source content, including new getBundlerRouteAnalysis and getRouteModuleAnalysis APIs. Route-chunk code generation and validity maps gain exported helpers (createEmptyRouteChunkByExportName, buildManifestChunkValidity, buildEnforceChunkValidity). A new src/performance.ts profiler measures per-operation wall-clock intervals using interval-union merging for concurrent operations. A new src/route-watch.ts module provides route topology watching for dev mode, emitting restart markers when route structure changes. Plugin options extend with logPerformance and lazyCompilation fields, wiring profiling and lazy-compilation forwarding into the plugin lifecycle. Virtual module registration switches from RspackVirtualModulePlugin to rspack.experiments.VirtualModulesPlugin. Prerender HTTP calls consolidate through a new withBuildRequest abort-controller helper. Extensive benchmark scripts (bench-builds.mjs, bench-client-entry-analysis.mjs, benchmark-yuku.mjs, fixture generator, comparison tools) and documentation files (methodology, audit, analysis, task plans, changesets) are added to support performance tracking and future optimization work.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the primary change: adding performance optimizations to the React Router plugin, which is the main focus of this substantial refactor.
Description check ✅ Passed The description is directly related to the changeset, providing a comprehensive summary of the optimization work including benchmarks, caching improvements, parser replacement, and new plugin options.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf/bundling-performance

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/plugin-utils.ts (1)

15-76: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle assignment patterns before allowing destructured exports through.

Line 603 uses this as the only guard for destructured exports, but AssignmentPattern falls through. export const { loader = fn } = obj would keep a server-only export in the client transform instead of failing closed.

Proposed fix
 export function validateDestructuredExports(
   id: AnyNode,
   exportsToRemove: readonly string[]
 ): void {
+  if (id.type === 'Identifier') {
+    if (exportsToRemove.includes(id.name)) {
+      throw invalidDestructureError(id.name);
+    }
+    return;
+  }
+
+  if (id.type === 'RestElement') {
+    validateDestructuredExports(id.argument, exportsToRemove);
+    return;
+  }
+
+  if (id.type === 'AssignmentPattern') {
+    validateDestructuredExports(id.left, exportsToRemove);
+    return;
+  }
+
   if (id.type === 'ArrayPattern') {
     for (const element of id.elements ?? []) {
       if (!element) {
         continue;
       }
 
-      if (
-        element.type === 'Identifier' &&
-        exportsToRemove.includes(element.name)
-      ) {
-        throw invalidDestructureError(element.name);
-      }
-
-      if (
-        element.type === 'RestElement' &&
-        element.argument.type === 'Identifier' &&
-        exportsToRemove.includes(element.argument.name)
-      ) {
-        throw invalidDestructureError(element.argument.name);
-      }
-
-      if (element.type === 'ArrayPattern' || element.type === 'ObjectPattern') {
-        validateDestructuredExports(element, exportsToRemove);
-      }
+      validateDestructuredExports(element, exportsToRemove);
     }
   }
 
   if (id.type === 'ObjectPattern') {
@@
 
       if (property.type === 'Property') {
-        if (
-          property.value.type === 'Identifier' &&
-          exportsToRemove.includes(property.value.name)
-        ) {
-          throw invalidDestructureError(property.value.name);
-        }
-
-        if (
-          property.value.type === 'ArrayPattern' ||
-          property.value.type === 'ObjectPattern'
-        ) {
-          validateDestructuredExports(property.value, exportsToRemove);
-        }
+        validateDestructuredExports(property.value, exportsToRemove);
       }
 
-      if (
-        property.type === 'RestElement' &&
-        property.argument.type === 'Identifier' &&
-        exportsToRemove.includes(property.argument.name)
-      ) {
-        throw invalidDestructureError(property.argument.name);
+      if (property.type === 'RestElement') {
+        validateDestructuredExports(property, exportsToRemove);
       }
     }
   }
 }

Also applies to: 589-604

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/plugin-utils.ts` around lines 15 - 76, The validateDestructuredExports
function does not handle AssignmentPattern nodes, which allows default values in
destructured exports to bypass validation. When destructuring with default
values like { loader = fn }, the AssignmentPattern case falls through without
checking if the identifier should be removed. Add checks for AssignmentPattern
in both the ArrayPattern and ObjectPattern iteration sections: for ArrayPattern
elements that are AssignmentPattern, validate the argument property if it's an
Identifier, and recursively validate if it's a nested pattern; similarly for
ObjectPattern properties with AssignmentPattern values, validate the argument
and recursively process nested patterns. This ensures server-only exports with
default values are properly caught and fail closed.
🧹 Nitpick comments (3)
scripts/bench-builds.mjs (2)

300-327: ⚡ Quick win

Add legend clarifying metric semantics in Markdown output.

The plugin operations table (lines 300-323) displays Total and Wall columns (line 304) but includes no legend explaining which metric accounts for concurrency. Per performance-timing-semantics-analysis.md, readers need clear guidance on which metric to use for comparisons.

Suggested addition before the table:

lines.push(
  '',
  `## Plugin Operations Breakdown`,
  '',
  '**Metric definitions:**',
  '- **Total (ms):** Cumulative wall time across all invocations (may overcount overlapping work)',
  '- **Wall (ms):** Wall-clock interval with overlap deduplication',
  '- **Max (ms):** Slowest single invocation (always accurate, no double-counting)',
  '',
  '| Environment | Operation | Count | Total | Wall | Max | Reports |',
  ...
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/bench-builds.mjs` around lines 300 - 327, The markdown table for
plugin operations lacks a legend explaining the metrics, making it unclear which
metric accounts for concurrency. Before pushing the table header with the
markdown column separators in the lines array, add explanatory text that defines
what each metric represents: Total should explain it is cumulative wall time
across invocations, Wall should explain it deduplicates overlapping work, and
Max should explain it is the slowest single invocation. Insert these legend
lines into the lines array before the existing table header markdown lines to
provide readers with clear guidance on metric semantics.

497-508: 💤 Low value

Benchmark failures are logged but allow process to continue.

Line 497-502 checks for any failed runs (non-zero status) and sets process.exitCode = 1, but the script continues after writeOutputs() at line 492. This means:

  1. Results file is written even if builds failed
  2. Process exits with code 1 only after all benchmarks complete
  3. If --fail-fast is NOT enabled, a single build failure doesn't stop the loop

This is generally correct (allow all benchmarks to run, report overall status), but may be confusing if a partial failure occurs — the output file will contain failed runs mixed with passed runs without a clear signal in the file itself.

Consider adding a failed flag to the result JSON so downstream comparison tools can detect and warn on partial failures.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/bench-builds.mjs` around lines 497 - 508, The benchmark results are
written to output regardless of whether failures occurred, making it difficult
for downstream tools to detect partial failures. Add a `failed` boolean flag to
the benchmark results object (the structure being passed to `writeOutputs()`)
that gets set to true when the condition at line 497 detects any runs with
non-zero status. This flag should be determined using the same logic that checks
if benchmarks.some(benchmark => benchmark.runs.some(run => run.status !== 0)),
allowing the output JSON file to include a clear indicator of whether the
benchmark run had any failures.
src/manifest.ts (1)

223-225: ⚡ Quick win

Consider failing the build on route analysis errors in production builds.

When getRouteModuleAnalysis throws, the error is logged but the build continues with empty exports. This produces an incorrect manifest where hasAction, hasLoader, etc., will all be false regardless of what the route actually exports. In dev mode this may be acceptable for hot-reload resilience, but in build mode (isBuild: true) this could cause silent runtime failures.

Consider re-throwing in build mode:

Suggested approach
       } catch (error) {
         console.error(`Failed to analyze route file ${routeFilePath}:`, error);
+        if (isBuild) {
+          throw error;
+        }
       }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/manifest.ts` around lines 223 - 225, The catch block for the
getRouteModuleAnalysis error logs the error but allows the build to continue,
resulting in an incorrect manifest with all route properties set to false. In
the catch block where the error is logged, add a check for isBuild mode and
re-throw the error if isBuild is true, ensuring the build fails on route
analysis errors in production builds while remaining resilient in dev mode. Keep
the current logging behavior but add the conditional re-throw after the
console.error call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@benchmarks/chunk-precompute-methodology.md`:
- Around line 69-71: The fenced code blocks at lines 69 and 341 in the markdown
file lack language tags, triggering the MD040 markdownlint rule. Add the
language tag `text` to both unlabeled code blocks by changing the opening triple
backticks from ``` to ```text for the array example and the metrics table
example to satisfy linting requirements.
- Around line 121-123: The documentation contains a hardcoded machine-specific
absolute path `/home/zack/projects/rsbuild-plugin-react-router` on line 122 that
will not be valid on other developers' machines or environments. Replace this
specific absolute path with a relative reference or generic placeholder that
conveys the same meaning (such as simply referring to "the repo root" or using a
relative path notation like `.`) to make the instructions portable across
different machines and development environments.

In `@package.json`:
- Line 85: Remove the unused rspack-plugin-virtual-module dependency from
package.json by deleting the line "rspack-plugin-virtual-module": "^1.0.1" since
virtual module handling has been migrated to
rspack.experiments.VirtualModulesPlugin. After removing this line from
package.json, also update the lockfile to remove any references to this package
by running the appropriate package manager command (npm install or yarn install
depending on which lockfile is being used).

In `@performance-timing-semantics-analysis.md`:
- Around line 118-122: Add a documentation comment or docstring to
`src/performance.ts` that explains the timing semantics of the performance
profiler fields. The documentation should clarify that totalMs is measured using
wall-clock deltas captured at record() calls and that due to concurrent async
transforms interleaving on the Node.js event loop, totalMs double-counts
overlapping wait time and can exceed the actual serial cost or even
compilerLifecycleMs. Include guidance that totalMs should be treated as an upper
bound, not precise attribution; explain which fields remain accurate under
concurrency (count, maxMs, slowest[], and compilerLifecycleMs); and advise users
to use the interval-union wallMs aggregate instead of totalMs for
concurrency-safe cost attribution across operations. You can paste the
ready-to-paste paragraph provided in the analysis document or adapt it to fit
your code style.

In `@route-analysis-duplication-audit.md`:
- Around line 1-170: The audit document contains stale line number references
that no longer match the current codebase—for example, cache declarations in
export-utils.ts are documented at lines 24–29 but actually exist at lines 49–55,
and getReactRouterManifestForDev is documented at line 110 but is actually at
line 147. Update all line number references throughout the document by
cross-referencing the specified files (export-utils.ts, manifest.ts, index.ts,
modify-browser-manifest.ts, build-manifest.ts) and the named functions and
caches (transformCache, exportNamesCache, routeModuleAnalysisCache,
getReactRouterManifestForDev, getRouteModuleAnalysis,
detectRouteChunksIfEnabled, etc.) to match their actual current locations.
Additionally, add a header note specifying the current commit hash or date when
the line numbers were verified to prevent future confusion.

In `@src/export-utils.ts`:
- Around line 183-197: The collectExportNames function is missing support for
TypeScript enum declarations, which are runtime exports that should be tracked.
Add a new else if condition after the FunctionDeclaration and ClassDeclaration
check to handle TSEnumDeclaration types. Follow the same pattern as
FunctionDeclaration and ClassDeclaration by checking if the declaration type is
'TSEnumDeclaration' and if declaration.id?.name exists, then add the name to the
exportNames set. This ensures exported enums are properly recorded alongside
other exported declarations.

In `@src/index.ts`:
- Around line 1365-1374: In the bundleId assignment on the line with the replace
method call, the regex pattern is incorrectly escaping the dot character. Change
the regex pattern from /\\.js$/ to /\.js$/ so that it correctly matches and
removes the actual `.js` suffix from the captured bundle ID string. This ensures
that when the module loader appends `.js` to virtual module requests, the
bundleId value is properly stripped (e.g., from `admin.js` to `admin`) so that
subsequent lookups in latestServerManifestsByBundleId work correctly.

In `@src/plugin-utils.ts`:
- Around line 784-829: The component wrapping logic in this export processing
block has gaps that skip certain valid component exports. First, remove the
condition checking expr.type !== 'ClassDeclaration' in the
ExportDefaultDeclaration branch (around line 791) so that default class exports
are also wrapped with the withComponentProps HOC. Second, add handling for
ExportNamedDeclaration statements that contain ExportSpecifier elements (such as
export { ErrorBoundary } or export { ErrorBoundary as Component }) to wrap those
component exports with the appropriate HOC. Third, extend the
FunctionDeclaration handling in the ExportNamedDeclaration branch to also handle
ClassDeclaration exports in the same way, ensuring that named class exports are
wrapped with the with*Props HOC just like function exports.
- Around line 550-566: The export removal logic in the loop that processes
program.body statements only handles ExportNamedDeclaration but ignores
ExportAllDeclaration (export * from '...'). This allows export * statements to
remain in the bundle even when they should be removed. Add a check for
statement.type === 'ExportAllDeclaration' alongside the existing
ExportNamedDeclaration check, and remove the entire ExportAllDeclaration
statement when its source module contains exports that are marked for removal in
the exportsToRemove set. This ensures export * statements don't leak server-only
exports into the web bundle.

---

Outside diff comments:
In `@src/plugin-utils.ts`:
- Around line 15-76: The validateDestructuredExports function does not handle
AssignmentPattern nodes, which allows default values in destructured exports to
bypass validation. When destructuring with default values like { loader = fn },
the AssignmentPattern case falls through without checking if the identifier
should be removed. Add checks for AssignmentPattern in both the ArrayPattern and
ObjectPattern iteration sections: for ArrayPattern elements that are
AssignmentPattern, validate the argument property if it's an Identifier, and
recursively validate if it's a nested pattern; similarly for ObjectPattern
properties with AssignmentPattern values, validate the argument and recursively
process nested patterns. This ensures server-only exports with default values
are properly caught and fail closed.

---

Nitpick comments:
In `@scripts/bench-builds.mjs`:
- Around line 300-327: The markdown table for plugin operations lacks a legend
explaining the metrics, making it unclear which metric accounts for concurrency.
Before pushing the table header with the markdown column separators in the lines
array, add explanatory text that defines what each metric represents: Total
should explain it is cumulative wall time across invocations, Wall should
explain it deduplicates overlapping work, and Max should explain it is the
slowest single invocation. Insert these legend lines into the lines array before
the existing table header markdown lines to provide readers with clear guidance
on metric semantics.
- Around line 497-508: The benchmark results are written to output regardless of
whether failures occurred, making it difficult for downstream tools to detect
partial failures. Add a `failed` boolean flag to the benchmark results object
(the structure being passed to `writeOutputs()`) that gets set to true when the
condition at line 497 detects any runs with non-zero status. This flag should be
determined using the same logic that checks if benchmarks.some(benchmark =>
benchmark.runs.some(run => run.status !== 0)), allowing the output JSON file to
include a clear indicator of whether the benchmark run had any failures.

In `@src/manifest.ts`:
- Around line 223-225: The catch block for the getRouteModuleAnalysis error logs
the error but allows the build to continue, resulting in an incorrect manifest
with all route properties set to false. In the catch block where the error is
logged, add a check for isBuild mode and re-throw the error if isBuild is true,
ensuring the build fails on route analysis errors in production builds while
remaining resilient in dev mode. Keep the current logging behavior but add the
conditional re-throw after the console.error call.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d22b41ff-52e3-4adc-ab3f-0b8c033cc04d

📥 Commits

Reviewing files that changed from the base of the PR and between 0b2b552 and f28c70b.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (46)
  • .changeset/bright-routes-run.md
  • .changeset/fast-routes-dance.md
  • .gitignore
  • benchmarks/README.md
  • benchmarks/chunk-precompute-methodology.md
  • benchmarks/manifest-performance-methodology.md
  • config/rslib.config.ts
  • package.json
  • performance-timing-semantics-analysis.md
  • route-analysis-duplication-audit.md
  • route-chunk-parse-traverse-analysis.md
  • scripts/bench-builds.mjs
  • scripts/bench-client-entry-analysis.mjs
  • scripts/benchmark-yuku.mjs
  • scripts/benchmark/fixture.mjs
  • scripts/compare-benchmarks.mjs
  • scripts/compare-client-entry-analysis.mjs
  • src/babel.ts
  • src/constants.ts
  • src/export-utils.ts
  • src/index.ts
  • src/manifest.ts
  • src/modify-browser-manifest.ts
  • src/performance.ts
  • src/plugin-utils.ts
  • src/route-artifacts.ts
  • src/route-chunks.ts
  • src/templates/entry.server.tsx
  • src/types.ts
  • src/virtual-modules.ts
  • task/lexer-route-export-triage.md
  • task/route-chunk-correctness-test-spec.md
  • task/route-chunk-precompute-plan.md
  • task/unified-route-module-analysis-cache-triage.md
  • tests/benchmark-fixture.test.ts
  • tests/export-utils.test.ts
  • tests/features.test.ts
  • tests/index.test.ts
  • tests/manifest-split-route-modules.test.ts
  • tests/manifest.test.ts
  • tests/performance.test.ts
  • tests/remove-exports.test.ts
  • tests/route-artifacts.test.ts
  • tests/route-chunks-cache.test.ts
  • tests/route-chunks.test.ts
  • tests/setup.ts

Comment thread benchmarks/chunk-precompute-methodology.md Outdated
Comment thread benchmarks/chunk-precompute-methodology.md Outdated
Comment thread package.json Outdated
Comment thread performance-timing-semantics-analysis.md Outdated
Comment thread route-analysis-duplication-audit.md Outdated
Comment thread src/export-utils.ts
Comment thread src/index.ts
Comment thread src/plugin-utils.ts
Comment thread src/plugin-utils.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/index.test.ts (1)

35-65: 💤 Low value

Consider adding test for explicit lazyCompilation: true.

The test suite covers the default behavior (implicit true), explicit object configuration, and explicit false, but not an explicit lazyCompilation: true. While the default test already validates true behavior, an explicit test would distinguish between "default true" and "user-provided true" for clarity.

🧪 Optional test case
it('should respect explicit lazy compilation true', async () => {
  const rsbuild = await createStubRsbuild({
    rsbuildConfig: {},
  });

  rsbuild.addPlugins([pluginReactRouter({ lazyCompilation: true })]);
  const config = await rsbuild.unwrapConfig();

  expect(config.dev.lazyCompilation).toBe(true);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/index.test.ts` around lines 35 - 65, Add a new test case to the test
suite that validates the behavior when lazyCompilation is explicitly set to the
boolean value true. Create a test following the same pattern as the existing
tests in tests/index.test.ts (using createStubRsbuild, addPlugins with
pluginReactRouter, and unwrapConfig), but pass lazyCompilation: true as a
boolean to distinguish between the default implicit true behavior and an
explicit user-provided true value. Assert that config.dev.lazyCompilation equals
true in this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/index.test.ts`:
- Around line 35-65: Add a new test case to the test suite that validates the
behavior when lazyCompilation is explicitly set to the boolean value true.
Create a test following the same pattern as the existing tests in
tests/index.test.ts (using createStubRsbuild, addPlugins with pluginReactRouter,
and unwrapConfig), but pass lazyCompilation: true as a boolean to distinguish
between the default implicit true behavior and an explicit user-provided true
value. Assert that config.dev.lazyCompilation equals true in this case.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 56e43e66-4109-4ba7-a17e-6ec8d1c71821

📥 Commits

Reviewing files that changed from the base of the PR and between f28c70b and f69bc2e.

📒 Files selected for processing (4)
  • scripts/benchmark/fixture.mjs
  • src/index.ts
  • src/types.ts
  • tests/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/types.ts
  • scripts/benchmark/fixture.mjs
  • src/index.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
examples/default-template/tests/e2e/dev-route-watch.test.ts (1)

124-131: ⚡ Quick win

Fail fast if the route insertion anchor is missing.

Line 127 silently leaves routes.ts unchanged when the anchor comment is absent, then the test fails later via timeout. Add an explicit anchor check so this fails immediately with a clear error.

Suggested hardening
     const routesConfig = readFileSync(routesConfigPath, 'utf8');
+    const anchor = '  // Docs section with nested routes';
+    if (!routesConfig.includes(anchor)) {
+      throw new Error(`Missing expected insertion anchor in ${routesConfigPath}`);
+    }
     writeFileSync(
       routesConfigPath,
       routesConfig.replace(
-        '  // Docs section with nested routes',
-        `${addedRouteConfigEntry}\n\n  // Docs section with nested routes`
+        anchor,
+        `${addedRouteConfigEntry}\n\n${anchor}`
       )
     );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/default-template/tests/e2e/dev-route-watch.test.ts` around lines 124
- 131, The code silently continues if the anchor comment is missing from the
routes configuration, causing test failures later with confusing timeout errors.
Before the writeFileSync call that uses routesConfig.replace(), add an explicit
check to verify that the anchor comment string '  // Docs section with nested
routes' exists in the routesConfig. If the anchor is not found using an
includes() or indexOf() check, throw an error immediately with a clear message
indicating the anchor comment is missing from the routes file, so failures are
caught and diagnosed right away instead of during test execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/route-watch.ts`:
- Around line 216-231: The runRescan function checks if closed is true at the
start, but if close() executes while awaiting readRouteDirectoryState (line
221), the function continues and can still call syncDirectoryWatchers and
touchRestartMarker, creating FSWatchers after shutdown. Add a check for the
closed flag immediately after the await completes and before calling
syncDirectoryWatchers to prevent post-close side effects and watcher leaks. This
same pattern should be applied to the other location mentioned at lines 254-263.

---

Nitpick comments:
In `@examples/default-template/tests/e2e/dev-route-watch.test.ts`:
- Around line 124-131: The code silently continues if the anchor comment is
missing from the routes configuration, causing test failures later with
confusing timeout errors. Before the writeFileSync call that uses
routesConfig.replace(), add an explicit check to verify that the anchor comment
string '  // Docs section with nested routes' exists in the routesConfig. If the
anchor is not found using an includes() or indexOf() check, throw an error
immediately with a clear message indicating the anchor comment is missing from
the routes file, so failures are caught and diagnosed right away instead of
during test execution.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bb9ee7a1-447c-449f-8ea4-b512716636b8

📥 Commits

Reviewing files that changed from the base of the PR and between fe68c84 and 59f88b8.

📒 Files selected for processing (7)
  • examples/default-template/playwright.config.ts
  • examples/default-template/tests/e2e/dev-route-watch.test.ts
  • src/index.ts
  • src/route-watch.ts
  • tests/index.test.ts
  • tests/route-watch.test.ts
  • tests/setup.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/index.test.ts
  • src/index.ts

Comment thread src/route-watch.ts
@arshad-yaseen

arshad-yaseen commented Jun 19, 2026

Copy link
Copy Markdown

I've been working on the precedence-aware printer in yuku, and it's been released in the latest yuku v0.5.39.

So I just had to mention it here when I saw aa33f69. Now preserveParens is no longer needed, you can disable it if needed, and the printer will still emit proper parens, independent of whether the paren nodes exist in the AST or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants